Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bash-env-json: fix path issue #358950

Merged
merged 1 commit into from
Dec 15, 2024
Merged

Conversation

jaredmontoya
Copy link
Contributor

@jaredmontoya jaredmontoya commented Nov 25, 2024

Things done

Fixed bash-env-json breaking if it tries to process a file that sets PATH.
This program cannot rely on PATH, therefore full paths must be used in this program all the time.
Upstream flake currently uses the same principle to build the package.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@jaredmontoya
Copy link
Contributor Author

@Aleksanaa can you please read this issue through and tell us how to go about this package in the long run?
tesujimath/bash-env-json#14

I want to know if there are any other nixpkgs packages that use nix code from the repo they are downloading to build the package and if it is even possible.

And if it is, is it allowed due to the fact that nix reviewers will have to review outside code that is imported before merging the updated derivation.

I think I already know the answer but I don't think I should be the one to say the final verdict to the maintainer of the program because I am just a package maintainer in nixpkgs who doesn't know all the rules, not an authority.

@Aleksanaa
Copy link
Member

There's actually a tool called resholve here: https://github.com/NixOS/nixpkgs/tree/nixos-unstable/pkgs/development/misc/resholve

it correctly identifies executables in scripts and replaces them with those provided in packages, and fails if the provided list isn't sufficient. However, it depends on Python2 thus is discouraged to use. But I still recommend you use it here, if possible.

@Aleksanaa
Copy link
Member

And if it is, is it allowed due to the fact that nix reviewers will have to review outside code that is imported before merging the updated derivation.

This is probably unrealistic. We will read the diff, but this does not mean that we can always fully understand them...

@Aleksanaa
Copy link
Member

And we have no responsibility to enforce a standard in other people's projects, which is needed in Nixpkgs from time to time. To put it bluntly, the writing format of most out-of-tree derivation is completely substandard (although some in-tree derivation is also like this)

@jaredmontoya
Copy link
Contributor Author

jaredmontoya commented Nov 26, 2024

@Aleksanaa Thank you for your explanation.

The main concern for us is, that if the bash-env-json maintainer adds another dependency to the script it will also have to be added to the derivation along with the update and it's worse than just simply changing the version number and hash in the derivation.

I didn't know about resholve and it sounds great except for the fact that it uses python2, but I think it won't exactly solve the issue because it still requires changes to inputs and package call parameters if new dependencies are introduced.

Also resholve failed with if ! source "$_path" >/dev/null 2>&4; then Can't resolve dynamic argument in 'source' on bash-env-json so it's out of the question if this error is not a result of my incompetency with resholve
here is what I tested it with:

{
  resholve,
  bash,
  lib,
  fetchFromGitHub,
  coreutils,
  gnused,
  jq,
  nix-update-script,
}:

resholve.mkDerivation rec {
  pname = "bash-env-json";
  version = "0.9.2";

  src = fetchFromGitHub {
    owner = "tesujimath";
    repo = "bash-env-json";
    rev = version;
    hash = "sha256-EYro4pMILnQFpXpFjdzSDuudhqC2EvysYMUmIOvesgo=";
  };

  installPhase = ''
    runHook preInstall

    install -Dm755 bash-env-json -t $out/bin

    runHook postInstall
  '';

  solutions = {
    default = {
      scripts = [ "bin/bash-env-json" ];
      interpreter = lib.getExe bash;
      inputs = [
        coreutils
        gnused
        jq
      ];
    };
  };

  passthru.updateScript = nix-update-script { };

  meta = {
    description = "Export Bash environment as JSON for import into modern shells like Elvish and Nushell";
    homepage = "https://github.com/tesujimath/bash-env-json";
    mainProgram = "bash-env-json";
    license = lib.licenses.mit;
    maintainers = with lib.maintainers; [ jaredmontoya ];
    platforms = lib.platforms.all;
  };
}

But as the docs say:

Fair warning: resholve does not aspire to resolving all valid Shell scripts. It depends on the OSH/Oil parser, which aims to support most (but not all) Bash. resholve aims to be a ~90% sort of solution.`

so it's expected.

I think I will just have to check a small section of the source code for new required dependencies before updating the package and add appropriate substituteInPlace which is not a big deal.

@jaredmontoya
Copy link
Contributor Author

jaredmontoya commented Dec 7, 2024

@Aleksanaa what is happening with https://github.com/NixOS/nixpkgs/pull/358950/checks?check_run_id=33519605117

Why does it take more than 13 days?

@jaredmontoya
Copy link
Contributor Author

jaredmontoya commented Dec 9, 2024

Finally, it's done.
Please merge.

@jaredmontoya
Copy link
Contributor Author

jaredmontoya commented Dec 15, 2024

@teto Can you merge this please? It will help to improve the nushell home-manager module.

@JohnRTitor JohnRTitor merged commit 9f1bd52 into NixOS:master Dec 15, 2024
52 checks passed
@jaredmontoya jaredmontoya deleted the bash-env-json-fix branch January 10, 2025 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants